Skip to content

fix: type of arg type should be of type type#265

Merged
nazarfil merged 2 commits into
mainfrom
fix/parameter-arg-types
May 20, 2025
Merged

fix: type of arg type should be of type type#265
nazarfil merged 2 commits into
mainfrom
fix/parameter-arg-types

Conversation

@yannforget

@yannforget yannforget commented May 19, 2025

Copy link
Copy Markdown
Member

When decorating a pipeline with @parameter, we use the type= argument to specify parameter type:

@pipeline(...)
@parameter(
    code="src_dhis2",
    type=DHIS2Connection,
    name="Source DHIS2 instance",
)
@parameter(
    code="dst_dataset",
    type=Dataset,
    name="Output dataset",
)
@parameter(
    code="org_units",
    type=str,
    multiple=True,
    name="Organisation units",
)
def my_pipeline():
   ...

We are not passing an instance of a class but the class/type itself. Therefore the annotated type of type should be of type type and not str | DHIS2Connection | Dataset | ....

@yannforget yannforget requested review from nazarfil and yolanfery May 19, 2025 15:58
@yannforget yannforget changed the title fix: type of arg should be of type type fix: type of arg type should be of type type May 19, 2025
yolanfery
yolanfery reviewed May 19, 2025
View reviewed changes

@yolanfery yolanfery left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type is too generic imo

I think this is correct instead, right ?

type: type[tr
    | int
    | bool
    | float
    | DHIS2Connection
    | IASOConnection
    | PostgreSQLConnection
    | GCSConnection
    | S3Connection
    | CustomConnection
    | Dataset],

@nazarfil

nazarfil commented May 19, 2025

Copy link
Copy Markdown

When decorating a pipeline with @parameter, we use the type= argument to specify parameter type:

I think we need more context to understand the need for this change

  • From the perspective of sdk itself it makes more sense to enforce typing to decrease number fo issues when pipeliens are built/executed and then na unkown error pops up, leading to a message in slack internal channel :D

@yannforget

yannforget commented May 20, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

@yolanfery I didn't know about the type[] syntax, indeed that's a lot better!

@nazarfil I agree it's better to enforce typing, but the current one is wrong. E.g. we are not passing type=4 as the current annotations suggest but type=int if we want the parameter to be an int. And the type of int is type, not int

This means that you cannot check an OpenHEXA pipeline with a static type checker such as pyright/pylance/mypy

@yannforget yannforget requested a review from yolanfery May 20, 2025 07:24
@nazarfil nazarfil merged commit d9e7421 into main May 20, 2025
4 checks passed
@nazarfil nazarfil deleted the fix/parameter-arg-types branch May 20, 2025 13:36
@blsqbot blsqbot mentioned this pull request May 20, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yolanfery yolanfery yolanfery approved these changes
+1 more reviewer
@nazarfil nazarfil nazarfil approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@yannforget @nazarfil @yolanfery